Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change food items list test to ignore order #144

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

emilie-robichaud
Copy link
Contributor

No description provided.

Copy link
Contributor

@donraab donraab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment inline for testing collection equality without regard for order.

"CHOCOLATE_CHIP cookie", "GLAZED donut");
assertEquals(expected, coffeeShopOrder.getFoodItemsForOrder());
List<String> expected = List.of("CHOCOLATE_CHIP cookie", "EVERYTHING bagel with HERB_GARLIC_CREAM_CHEESE", "GLAZED donut");
assertThat(coffeeShopOrder.getFoodItemsForOrder()).hasSameElementsAs(expected);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to use a Bag from Eclipse Collections to test that two Lists contain the same elements without requiring the same order. Since Eclipse Collections is already a dependency in other modules in this repo, I would prefer Bag be used here to test element only equality instead of adding a new library dependency.

The code would look as follows:

assertEquals(Bags.mutable.of(expected), Bags.mutable.of(coffeeShopOrder.getFoodItemsForOrder()));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Don! I think assertThat.hasSameElementsAs() reads easier for beginners, and people may not be familiar with Bag

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new dependency (with other transitive dependencies) for a single assertion method does not make sense to me. The kata is intended to teach new Java features, not to teach a third-party assertion library. Add a method named "assertHasSameElementsAs(List, List)" and the readability is similar.

public void assertHasSameElementsAs(List<?> expected, List<?> actual)
{
    assertEquals(Bags.mutable.of(expected), Bags.mutable.of(actual));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @emilie-robichaud, Bag is an advanced topic and Eclipse Collections is itself not covered as part of this kata and it would not make sense to add it in tests just for assertions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prathasirisha @emilie-robichaud The method assertHasSameElementsAs above can be implemented using JDK collections only then.

<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>3.24.2</version>
</dependency>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add test to this dependency

@emilie-robichaud emilie-robichaud force-pushed the master branch 2 times, most recently from 9834d40 to 16f6276 Compare September 19, 2023 15:47
Copy link
Contributor

@donraab donraab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@donraab donraab merged commit 1b38ad9 into BNYMellon:master Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants